raidz: fix space accounting after expansion - #18324

@amotin — following your feedback on #18170, I've implemented the per-block birth txg approach you described, using BP_GET_PHYSICAL_BIRTH() and the existing reflow_node_t AVL tree. Metaslab self-consistency is preserved.
Would appreciate your thoughts!

Maybe it would be worthwhile to add some explicit tests of this functionality, to verify that the amount of data required to fill a pool is approximately equal to the remaining freespace that is reported?

For example, create a pool and then do a series of large writes interspersed with raidz expansions, while verifying that the size of the write, the amount of freespace remaining, and the amount of space consumed before/after are all within some tolerance of each other? It might also be worthwhile doing a version with where you do a ZFS rewrite each time inbetween expansions.

@owlshrimp Good idea — added raidz_expand_009_pos (multi-expansion with writes between each step) and raidz_expand_010_pos (rewrites between expansions + fills pool to verify freespace accuracy).
All passing locally!

...verifying that the size of the write, the amount of freespace remaining, and the amount of space consumed before/after are all within some tolerance of each other?

I suppose it would be more correct to say that the reported freespace before vs after should be approximately equal to the size of the write (with compression turned off, anyway). The various cases of "how much space should this have taken" with different stripe sizes makes my head hurt though, especially if you delete old data. Deleting old data is probably another area where tests verifying expected behavior would be useful.

Mh, the two new tests cover both.
Test 009 does multi-expansion with writes at each step, checking that freespace decrease tracks allocated increase (not the logical write size directly — on RAIDZ those differ due to parity).

Test 010 does delete+rewrite cycles between expansions, then fills the pool to verify reported freespace actually matches writable capacity.

Let me know if you think that covers it or if you had something else in mind.

Let me know if you think that covers it or if you had something else in mind.

Reading through the testing strategies, I think this probably covers everything I can think of offhand. Maybe there are weird edgecases with mixes of pre and post expansion data, but 09 seems like at least a very good sanity check.

What would we expect to happen to the total figures if some quantity of pre-expansion data is deleted? I presume the freespace would go up by a bit more than the amount of data deleted, due to the difference in parity ratio?

What would we expect to happen to the total figures if some quantity of pre-expansion data is deleted? I presume the freespace would go up by a bit more than the amount of data deleted, due to the difference in parity ratio?

Right, pre-expansion blocks use more raw space per logical byte (worse parity ratio), so deleting them frees raw capacity that under the new geometry maps to more usable space. That's the convergence the commit message describes: as old blocks get deleted or rewritten, accounting gradually aligns with the new geometry.
Test 010 exercises exactly that path.

I suppose every time you do an expansion (with these changes) the pre-expansion blocks belonging to files and snapshots look like they consume slightly more usable space than expected for their logical size.

Have you double checked things like the amount of space reported as consumed by old snapshots whose files have been deleted from the current view makes sense in some way? (eg. in zfs list -t all) Just thinking of potential edge cases.

Yes, pre-expansion blocks genuinely cost more raw space per logical byte (worse parity ratio), so that's expected and correct.
The accounting stays self-consistent because bp_get_dsize_sync() always uses the block's physical birth txg to look up the deflation ratio, so a given block always produces the same dsize regardless of when you compute it.

Good call on snapshots though, I don't have explicit test coverage for that. I'll add one just after dinner (UTC+1 here!)

Also added an export/import check to test 008 — vdev_deflate_ratio_current isn't persisted on disk, it's recomputed during vdev_open() on import. So if that recomputation ever regressed, all accounting would silently break after a reboot or pool reimport. Now verified it survives the cycle.

I guess every time you delete a pre-expansion file that exists in a snapshot, the snapshot grows by the same slightly-more-than-logical amount of raw size as the file used to occupy?

Yes exactly, the snapshot's used increases by the dsize of those blocks, which uses the old deflation ratio (via birth txg). So it reflects the actual raw cost of holding pre-expansion data. And test 11 already verifies this.

Assuming one rewrote all the data (and didn't keep any snapshots) are there any lingering differences between an expanded array and one that was constructed natively, given this patchset?

No, from this patchset's perspective there are no lingering differences. Once all data is rewritten under the new geometry, the per-block deflation ratios all reflect the current layout (identical to native), and the SPA-level capacity correction produces the same reported size as a natively constructed array.

The expansion feature itself does leave persistent metadata on disk (e.g., the raidz_expansion feature flag, expansion history), but that is unrelated to this patchset and does not affect space accounting or I/O behavior.

This also follows up on the "time dependent deflate ratio" concept originally described by @ahrens in #12225 (comment)

How do I apply this patch? Or is there a big chance this will find its way into an official openzfs release soon?

How do I apply this patch? Or is there a big chance this will find its way into an official openzfs release soon?

This patch is not ready yet and is still work in progress. If you attempt to apply it you are putting your data at risk.

After it's complete and passes review it will be merged into the main branch, and presumably will ship in a new release soon after that.

@amotin, @behlendorf

Hey, just wanted to follow up. I rebased and repushed yesterday so it should be clean on top of current main.
All review comments have been addressed as well so far.
Let me know if there's anything I can do to help move it forward!

I am not a ZFS developer, but it makes sense not to squash commits during the review phase at least, since it's hard to understand what changed in the last commit.

I am not a ZFS developer, but it makes sense not to squash commits during the review phase at least, since it's hard to understand what changed in the last commit.

The contributing guidelines seems to disagree with you:

@amotin @behlendorf @ahrens This seems like a really useful series and a final missing piece for raidz expansion. Is there someone who might have the time to review this?

@Skountz would you mind rebasing this again and addressing the spa_feature_t enum reordering and related abi updates while you're at it. I'll try and make time for a full review this week or next.

The post-rebase checkABI passed, and everything is in order, I'm running the tests on a VM while the CI is doing its job just in case, but I guess we're good for the review once those are passing!

It seems some qemu checks are failing?

Another rebase on master should sort out the CI failures.

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.

#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Meanwhile I'm working through behlendorf's review here: making the feature READONLY_COMPAT instead of MOS. Testing now.

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.

#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Does that mean this work might close that corner case, or would more work likely be required to fix that particular issue?

that issue with dedup looks......hairy. I hope it doesn't have bearing against this work, but I wouldn't bet against it.

@owlshrimp no bearing here, they're complementary. That mixed-epoch case (a dedup entry born before expansion, extended with a post-expansion DVA) is exactly what this PR's per-block accounting handles: each DVA's size comes from its own stored ASIZE and the block's physical birth, both immutable, so a block is freed with the same deflated size it was born with. No leak.
#18826 fixes a different path, where RAIDZ uses that single birth to pick the stripe width when reading the DVA. That's an I/O safety issue, not an accounting one.

Does that mean this work might close that corner case, or would more work likely be required to fix that particular issue?

No, and it's not meant to. This PR is purely about space accounting; it guarantees a block is freed with the same deflated size it was born with, per DVA. It never touches the read path, so it does nothing to prevent the wrong-geometry read that #18826 addresses. That mixed-epoch block pointer still shouldn't exist, and #18826 is what stops it from being created (and contains pools that already carry one). The two are independent and both needed; neither closes the other.